fix(server): encode V2 response IntegrityIds - #818
Conversation
Co-Authored-By: Codex <noreply@openai.com>
|
@bvanelli Could you review the V2 emulator response behavior here, particularly selecting the read/write counter and appending the IntegrityId without changing the V1 payload layout? The conflict repair also preserves the newer symbolic-read TCP-reset emulation. The refreshed head passes 1,779 tests, the complete pre-commit suite, and the package build. |
# Conflicts: # tests/test_s7_v2.py
|
@ale-rinaldi you have detailed V2/TLS observations from the 1507S and 1211C. Could you review the server response layout here against your real-PLC captures or traces, particularly whether the read/write IntegrityId selection and appended counter match V2 responses without changing the V1 payload shape?\n\nThis is primarily a capture/protocol review; it does not require exposing project data or pointing a real PLC at the emulator. |
| response = self._build_error_response(seq_num, req_session_id, function_code) | ||
| rst = False | ||
|
|
||
| if self._protocol_version >= ProtocolVersion.V2 and req_session_id != 0: |
There was a problem hiding this comment.
Here, it appends the integrity_id to every function. In theory, this could also apply to _build_error_response and _handle_init_ssl. I'm not sure those use need the integrity id.
There was a problem hiding this comment.
Good question—the generic placement needed an explicit audit. InitSSL cannot receive this suffix because it runs with a zero request session id, so the active-session guard skips it. An unsupported request made inside an established V2 session does use the write IntegrityId, matching the client, which advances its write counter for every non-read request.
While checking this, I found a nearby real bug: GetVarSubStreamed still emitted its legacy zero field before the generic V2 path appended the current counter, producing two IntegrityIds. The updated head 66afe4f fixes that and adds regressions covering exactly one V2 substreamed IntegrityId, no InitSSL IntegrityId, and the write counter on an in-session error response. The merged tree passes all pinned hooks, source/wheel builds, and the full local suite (2,005 passed, 82 skipped). Thanks for prompting the broader check.
2 participants
Summary
The branch now includes current
masterthrough a merge commit; its previous conflicts are resolved without rewriting history.Verification
uv run --frozen --extra test --extra s7commplus pytest -q(1779 passed, 82 skipped)uv run --frozen pre-commit run --all-filesuv build --no-sourcesFixes #802